Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

remove dbs no one uses #116

Closed
wants to merge 7 commits into from

Conversation

faddat
Copy link
Contributor

@faddat faddat commented Jan 7, 2024

  • add pebbledb
  • use latest pebble
  • added a ton of test rigor and fixed linting
  • if always false we don't need the param
  • really we don't need that
  • remove databases no one uses

@faddat faddat requested a review from a team as a code owner January 7, 2024 10:33
Copy link

codecov bot commented Jan 7, 2024

Codecov Report

Attention: 107 lines in your changes are missing coverage. Please review.

Comparison is base (d03785d) 68.59% compared to head (a574117) 60.70%.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #116      +/-   ##
==========================================
- Coverage   68.59%   60.70%   -7.89%     
==========================================
  Files          27       18       -9     
  Lines        2124     1438     -686     
==========================================
- Hits         1457      873     -584     
+ Misses        592      506      -86     
+ Partials       75       59      -16     
Files Coverage Δ
goleveldb.go 59.84% <100.00%> (ø)
test_helpers.go 100.00% <100.00%> (ø)
util.go 36.66% <100.00%> (ø)
db.go 35.00% <50.00%> (ø)
remotedb/iterator.go 30.00% <0.00%> (ø)
remotedb/remotedb.go 39.39% <0.00%> (ø)
remotedb/batch.go 58.00% <25.00%> (-3.71%) ⬇️
remotedb/grpcdb/server.go 0.00% <0.00%> (ø)
memdb_iterator.go 90.42% <33.33%> (-2.99%) ⬇️
memdb.go 69.64% <16.66%> (-1.27%) ⬇️
... and 1 more
Files Coverage Δ
goleveldb.go 59.84% <100.00%> (ø)
test_helpers.go 100.00% <100.00%> (ø)
util.go 36.66% <100.00%> (ø)
db.go 35.00% <50.00%> (ø)
remotedb/iterator.go 30.00% <0.00%> (ø)
remotedb/remotedb.go 39.39% <0.00%> (ø)
remotedb/batch.go 58.00% <25.00%> (-3.71%) ⬇️
remotedb/grpcdb/server.go 0.00% <0.00%> (ø)
memdb_iterator.go 90.42% <33.33%> (-2.99%) ⬇️
memdb.go 69.64% <16.66%> (-1.27%) ⬇️
... and 1 more

@faddat
Copy link
Contributor Author

faddat commented Jan 7, 2024

I am okay with codecov propblems. We should rip it out anyhow.

@faddat faddat closed this Jan 7, 2024
@faddat faddat reopened this Jan 9, 2024
@jmalicevic
Copy link
Contributor

Hi @faddat , thank you for the benchmarks and insights. As @adizere stated, we don't want to abruptly remove support for all other databases as we have a very large userbase still using them. We have plans to converge to one DB backend in the future but this requires an extensive study on what users are using. I would therefore separate the concerns of adding Pebble and removing other backends and keep in the PR only the addition of Pebble.

@faddat
Copy link
Contributor Author

faddat commented Jan 9, 2024

ok, I will change this to draft in that case.

@faddat faddat marked this pull request as draft January 9, 2024 20:55
@faddat
Copy link
Contributor Author

faddat commented Jan 9, 2024

Just so you know, somebody has written a universal converter. I just don't know who. It may have been The team that starts on a p but isn't persistence but I always forget their name

@faddat
Copy link
Contributor Author

faddat commented Jan 14, 2024

personally I dont care about tenths of a percent of on codecov. Please let me know if it matters.

@faddat
Copy link
Contributor Author

faddat commented Jan 14, 2024

I think that this PR remains a good idea.

@faddat faddat marked this pull request as ready for review January 14, 2024 21:37
@adizere
Copy link
Member

adizere commented Jan 15, 2024

The PR would be a lot better without this commit a574117. It's difficult to review right now. Can you revert that commit?

@faddat
Copy link
Contributor Author

faddat commented Jan 15, 2024

100%

@faddat
Copy link
Contributor Author

faddat commented Jan 15, 2024

closing in favor of #115

@faddat faddat closed this Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants